wow thanks!! this looks pretty good.

1. please test on iOS. you can buy an old iOS device for about $150.

2. use shorter names. serviceIndex,secondaryServiceIndex, characteristicIndex

3. iOS: hashes are not guaranteed to be unique. so your code will not always work. you should just use it's array location, i.e. index. e.g.

for (int characteristicIndex = 0; characteristicIndex < characteristics.length; characteristicIndex++);

4. in dart, the index should default to 0

class BluetoothCharacteristic {
  final DeviceIdentifier remoteId;
  final Guid serviceUuid;
  final int serviceIndex;
  final Guid? secondaryServiceUuid;
  final int? secondaryServiceIndex;
  final Guid characteristicUuid;
  final int characteristicIndex;

  BluetoothCharacteristic({
    required this.remoteId,
    required this.serviceUuid,
    this.serviceIndex = 0,
    this.secondaryServiceUuid,
    this.secondaryServiceIndex = 0,
    required this.characteristicUuid,
    required this.characteristicIndex = 0,
  });

1. please test on iOS. you can buy an old iOS device for about $150.

Is it possible to build for iOS on a Windows machine? Anyway I've since had a colleague confirm that this is working as expected on iOS!

2. use shorter names. serviceIndex,secondaryServiceIndex, characteristicIndex

Currently this is named after the Android instanceId. I was thinking it could be better to make a Guid/int pair type, perhaps called BluetoothAttribute, and use it everywhere where we have the two? Can also provide getters to still be able to access the uuid property directly.

3. hashes are not guaranteed to be unique. so your code will not always work. you should just use it's array location, i.e. index. e.g.

for (int serviceIndex = 0; serviceIndex < services.length; serviceIndex++);

Can we be certain that the order will never change? Based on https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/10.12/CoreBluetooth.framework/CBCharacteristic.h I suspect the internal hash function is just combining the handle and the uuid - so unless there are hundreds of characteristics with the same UUID it is highly unlikely they would collide.

4. in dart, the index should default to 0

class BluetoothCharacteristic {
  final DeviceIdentifier remoteId;
  final Guid serviceUuid;
  final int serviceIndex;
  final Guid? secondaryServiceUuid;
  final int? secondaryServiceIndex;
  final Guid characteristicUuid;
  final int characteristicIndex;

  BluetoothCharacteristic({
    required this.remoteId,
    required this.serviceUuid,
    this.serviceIndex = 0,
    this.secondaryServiceUuid,
    this.secondaryServiceIndex = 0,
    required this.characteristicUuid,
    required this.characteristicIndex = 0,
  });

Sure - will do unless you like my proposal for BluetoothAttribute.

Is it possible to build for iOS on a Windows machine?

ah good point. I don't think so.

Currently this is named after the Android instanceId. I was thinking it could be better to make a Guid/int pair type, perhaps called BluetoothAttribute, and use it everywhere where we have the two? Can also provide getters to still be able to access the uuid property directly.

I think just keep it simple. What you have now is good.

I just dont like the name. it's longer, more complicated, not available on iOS. But I understand it makes sense if this feature was android-only.

Can we be certain that the order will never change?

Yes. They guarantee order on iOS, because it's important for things like this.

Have completed the changes and will have colleague verify iOS again.

The [characteristic.service.characteristics indexOfObject:characteristic] everywhere feels somewhat convoluted but I believe this will achieve the desired effect.

Also implemented primary/secondary service support for descriptors.

I have been refactoring secondary services, so we are going to have a merge conflict, sorry about that.

I'm about to push the changes to master branch.

here is the commit.

2064c18

please read this comment for context: #948 (comment)

No worries, I was actually considering proposing that exact change so I am glad to see it! Will refactor tomorrow.

also, you'll need to mirror some of the changes I made.

1. always use the same order. I use, in order: remoteId, svcUuid, chrUuid, descUuid, primarySvcUuid

for you, you should do: remoteId, svcUuid, svcIndex, chrUuid, chrIndex, descUuid, primarySvcUuid, primarySvcIndex

2. you need to update code that makes keys

                    String key = remoteId + ":" + serviceUuid + ":" + characteristicUuid + ":" + descriptorUuid + ":" + primaryServiceUuid;
                    ```

e..g `mWriteDesc`, `mWriteChr,` and the same for iOS: `writeChrs`, `writeDesc`

also make sure you update this code, to check that the indexes are equal

  // get known service
  BmBluetoothService? get _bmsvc {
    if (FlutterBluePlus._knownServices[remoteId] != null) {
      for (var s in FlutterBluePlus._knownServices[remoteId]!.services) {
        if (s.serviceUuid == serviceUuid) {
          if (s.primaryServiceUuid == primaryServiceUuid) {
            return s;
          }
        }
      }
    }
    return null;
  }

  /// get known characteristic
  BmBluetoothCharacteristic? get _bmchr {
    if (_bmsvc != null) {
      for (var c in _bmsvc!.characteristics) {
        if (c.characteristicUuid == uuid) {
          return c;
        }
      }
    }
    return null;
  }

Although having had a quick look at it I'm not sure I expected includedServices to be removed. I think that was a good representation of the service hierarchy as it is defined in GATT.

Could we make it bi-directional by retaining includedServices alongside primaryServiceUuid? It would just involve initializing all BluetoothService instances first with primaryServiceUuid assigned, then on a 2nd pass populating the includedServices.

@nebkat , I added it back: a42ca5d

I just temporarily removed it.

The underlying representation changed.

Now instead of returning a List<List<BmBluetoothService>>, we now return List<BmBluetoothService> + added a primaryServiceUuid field.

The more important change is that I wanted to switch from secondaryServiceUuid, to primaryServiceUuid. It's simpler to think about and now all code can use the primaryService concept instead of mixing and matching primary/secondary + includedServices as we move around the codebase.

Also, iOS and Android don't use secondaryServiceUuid either. They just call it serviceUuid + a isSecondary flag. So now our API matches theirs.

I'm thinking about your change more.

I think you're right.

on ios youre probably right it's better to use the hash. but we could also make our own simple hash too if possible. that's probably better.

I'm also wondering if we should remove primaryServiceUuid from the public API.

tbh I'm not sure the best way to represent all of this stuff, especially with the addition of handles. But I know secondaryServiceUuid was not the right solution.

looking at this, they call it handle as well

@interface CBCharacteristic : NSObject {
    CBUUID * _UUID;
    NSArray * _descriptors;
    NSNumber * _handle;
    BOOL  _isBroadcasted;
    BOOL  _isNotifying;
    CBPeripheral * _peripheral;
    long long  _properties;
    CBService * _service;
    NSData * _value;
    NSNumber * _valueHandle;
}

and in the BLE spec as well, they use handles

Service UUID: 0x180F (Battery Service)
  - Handle: 0x0001
  - Attribute: Included Service
    - Handle: 0x0002 (reference to Environmental Sensing Service)
    - UUID: 0x181A (Environmental Sensing Service)

Service UUID: 0x181A (Environmental Sensing Service)
  - Handle: 0x0002
  - Characteristics: ...

But I think we should keep it index.

better idea, what if we instead added a index to Guid?

class Guid {
  final List<int> bytes;
  final String index = 0;

string representation:

if (index != 0) {
   // 0000-0000-1000-8000-00805f9b34fb:$index
}

normally it will be set to 0.

and only in cases where there are dupicate services, or duplicate characteristics in a service, or duplicate descriptors within a charachteristic, will it have a value.

I think we should try to make it work with an index, first. That seems best to me.

better idea, what if we instead added a index to Guid?

I would have to disagree there. A Guid class should represent exactly that, not a compound of Guid and index. If we wanted to go down that route I think a BluetoothAttribute class would make more sense. Also worth noting a descriptor for example doesn't support multiple of the same UUID.

However, if we are open to wider changes I would propose the following:

GATT does internally use handle to identify specific entries in the table, but both Android and iOS hide this from us and expect us to rely on references to specific instances of BluetoothGattCharacteristic/CBCharacteristic (and service equivalent). In other words, the index/instanceId/hash are all implementation details at the Java/Objective-C level.

Consequently it would make sense that we hide this implementation detail from the user in Dart, and rely on the same principle of identifying by specific instances of BluetoothCharacteristic.

Thus I would suggest the following structure:

class BluetoothAttribute {
    final DeviceIdentifier remoteId;
    final Guid uuid;
    final int index = 0;
}

class BluetoothService extends BluetoothAttribute {
    final bool isPrimary;
    final BluetoothService? parent;
    final List<BluetoothCharacteristic> characteristics = [];
    final List<BluetoothService> includedServices = [];

    BluetoothService._({
        required super.remoteId,
        required super.uuid,
        required super.index,
        required this.isPrimary,
        this.parent,
    });

    void _addIncludedService(BluetoothService service) {
        includedServices.add(service);
    }

    void _addCharacteristic(BluetoothCharacteristic characteristic) {
        characteristics.add(characteristic);
    }
}

class BluetoothCharacteristic extends BluetoothAttribute {
  final BluetoothService service;
  final List<BluetoothDescriptor> descriptors;

  BluetoothCharacteristic._({
    required super.remoteId,
    required super.uuid,
    required super.index,
    required this.service,
  });

  void _addDescriptor(BluetoothDescriptor descriptor) {
    descriptors.add(descriptor);
  }
}

class BluetoothDescriptor extends BluetoothAttribute {
  final BluetoothCharacteristic characteristic;

  BluetoothDescriptor._({
    required super.remoteId,
    required super.uuid,
    required this.characteristic
  });
}

Upon receiving the discovered services we would perform the two-stage mapping: initializing all the classes with their parent service/characteristic, and then adding them to the respective lists of children.

This would have the following benefits:

• Significant code simplification
• Lightweight - all classes are just holding their own UUID/index and potentially references to their parents/children
• Closely mirrors the GATT representation of it
• Also works similarly to native Android/iOS implementation
• No longer need to reference service, primary service, characteristic etc at all levels of the tree
• Still able to access all that information e.g. descriptor.characteristic.service.parent.uuid
• Can provide getters to simplify this e.g. get service => characteristic.service, get parentService => characteristic.service.parent
• Can provide getters to simplify transition/deprecation e.g. get serviceUuid => service.uuid

If this is something we would be open to I would gladly provide a PR.

I like it. However, I would use the same existing names where possible. i.e. characteristicUuid instead of just uuid.

we already have a uuid convenience accessor.

In fact, I think we can implement this with basically no breaking changes to the API?

class BluetoothCharacteristic extends BluetoothAttribute {

  final BluetoothAttribute _service;
  final BluetoothAttribute? _primaryService;

  Guid get serviceUuid => _service.uuid;
  Guid get characteristicUuid => uuid;
  Guid? get primaryServiceUuid = > _primaryService.uuid;

  final List<BluetoothDescriptor> descriptors;

  BluetoothCharacteristic({
    required super.remoteId,
    required serviceUuid,
    required characteristicUuid,
    this.primaryServiceUuid,
    serviceIndex = 0,
    characteristicIndex = 0,
    primaryServiceIndex = 0
  }) super.uuid = characteristicUuid, 
    _service =  BluetoothAttribute(remoteId, serviceUuid, serviceIndex) ;

  void _addDescriptor(BluetoothDescriptor descriptor) {
    descriptors.add(descriptor);
  }
}

To be honest, I like your approach more. But I'd rather not have to go "2.0" with the API.

Let's push a "1.0" compatible version first.

Or maybe we can go 2.0 if we provide 1.0 compatible constructors as well.

but the more a think about it. I think the clean 1.0 way is to not have a BluetoothAttribute class.

just add indexes to the existing classes. put them all together not interleaved, looks nicer:

  final DeviceIdentifier remoteId;
  final Guid serviceUuid;
  final Guid characteristicUuid;
  final Guid? primaryServiceUuid;

  final int serviceIndex;
  final int characteristicIndex;
  final int? primaryServiceIndex;

However, Internally, I still like the idea of using the uuid:$index strings. For example, in the Bm* classes, and in the locateCharacteristic functions. There's no need to add a bunch more arguments everywhere. It just gets too ugly / complicated.

I would have to disagree there. A Guid class should represent exactly that, not a compound of Guid and index. If we wanted to go down that route I think a BluetoothAttribute class would make more sense. Also worth noting a descriptor for example doesn't support multiple of the same UUID.

a guid is supposed to be globally unique. So I think it still makes more sense than what we have today. Obviously it was poorly named in the original flutter_blue.

I think adding index to Guid is the best "1.0" compatible way to add this feature. Open to alternatives. small breaking are changes okay, but maximum 1 or 2.

I like it. However, I would use the same existing names where possible. i.e. characteristicUuid instead of just uuid.

BluetoothCharacteristic.characteristicUuid seems a bit redundant which is why I was thinking a deprecated getter would make sense. Similarly BluetoothCharacteristic.service.uuid is easier to read than BluetoothCharacteristic.serviceUuid and only costs 1 extra character.

Sure it's mostly obvious what a serviceUuid is to a characteristic, but with a descriptor it almost implies that the descriptor is somehow tied to the service, which it is not directly - having the object path makes the relationships clearer. This is mostly an implementation detail because we needed that data to identify the descriptor in the plugin.

Or maybe we can go 2.0 if we provide 1.0 compatible constructors as well.

Which of these classes should actually be constructible by the user? If I'm not mistaken none of these characteristics/services will work unless they are discovered in the low level API, so a user should really get a reference to our instance of the BluetoothCharacteristic rather than creating their own.

It would be great if we could save a round of development and go straight to 2.0, I don't think any of these changes would involve much migration - if any.

However, Internally, I still like the idea of using the uuid:$index strings. For example, in the Bm* classes, and in the locateCharacteristic functions. There's no need to add a bunch more arguments everywhere. It just gets too ugly / complicated.

Internally I don't mind, we could even reference everything in the format: $PRIMARY_SERVICE_UUID:$PRIMARY_SERVICE_INDEX/$SERVICE_UUID:$SERVICE_INDEX/$CHARACTERISTIC_UUID:$CHARACTERISTIC_INDEX/$DESCRIPTOR_UUID. Would need a little bit more thought but you get the idea. We do exchange a lot of mapped data that is probably unnecessary if we could directly identify the service/char/descriptor. It is also duplicated all over the Java/Objective-C classes.

If you are open to it there are many ways we could significantly simplify the code that communicates between Dart/Java/Objective-C.

a guid is supposed to be globally unique. So I think it still makes more sense than what we have today. Obviously it was poorly named in the original flutter_blue.

This is a confusing concept within GATT but a UUID is meant to uniquely identify the type of the data, not the characteristic itself. If we attempt to correct this by changing what a Guid is (which is just another name for UUID) we will add to the further confusion - instead I think we should educate users on the distinction.

All fair. Just want it to be 1.0 api compatible, with no more than 1 or 2 small breaking changes.

Any suggestions? I think modifying Guid is the easiest way to support this feature in 1.0 friendly way.

BluetoothCharacteristic.characteristicUuid seems a bit redundant which is why I was thinking a deprecated getter would make sense. Similarly BluetoothCharacteristic.service.uuid is easier to read than BluetoothCharacteristic.serviceUuid and only costs 1 extra character.

not necessarily. If it was just elm.uuid, it's no longer clear. its a minor detail. But I have no plans to deprecate characteristicUuid. Having both is better. In fact, the original code just used uuid, but I know from experience - it got annoying fast.

For example, at quick glance, what does .where((c) => c.uuid == uuid) refer to?

It probably took you about 0.2 seconds to find out. But when it's named characteristicUuid, its 0.0 seconds. Navigating a code base with dozens of instances just like this, becomes annoying fast.

By using characteristicUuid, there is a clear path from dart to native, all using the same variable name. Greppability is an underrated code metric

Having both is the best.

All fair. Just want it to be 1.0 api compatible, with no more than 1 or 2 small breaking changes.

Any suggestions? I think modifying Guid is the easiest way to support this feature in 1.0 friendly way.

Let me make a PR this weekend, I think it will be possible with 0 or 1 breaking changes.

not necessarily. If it was just elm.uuid, it's no longer clear. its a minor detail. But I have no plans to deprecate characteristicUuid. Having both is better. In fact, the original code just used uuid, but I know from experience - it got annoying fast.

If elm.uuid is unclear it is elm which is the issue - I tend to name my characteristics heartbeatChar. But I take your point, can leave characteristicUuid as a getter.

It probably took you about 0.2 seconds to find out. But when it's named characteristicUuid, its 0.0 seconds. Navigating a code base with dozens of instances just like this, becomes annoying fast.

These instances will practically all be eliminated - there will be a single place in the code where we map from the $SERVICE_UUID:$SERVICE_INDEX/$CHARACTERISTIC_UUID:$CHARACTERISTIC_INDEX style string to an instance of BluetoothCharacteristic and nowhere else internally will care about UUIDs at all.

On further investigation a lot of things seems strangely overcomplicated with all of the BluetoothXXXX classes acting more like tokens providing views into maps within the main class. Is there a reason why for example a BluetoothCharacteristic doesn't maintain it's own value, list of descriptors, etc rather than pulling from _knownServices?

yes. because users can instantiate these objects themselves. This function has always been part of the API, and users have used it since day 1.

  BluetoothCharacteristic.fromProto(protos.BluetoothCharacteristic p)
      : uuid = new Guid(p.uuid),
        deviceId = new DeviceIdentifier(p.remoteId),
        serviceUuid = new Guid(p.serviceUuid),
        secondaryServiceUuid = (p.secondaryServiceUuid.length > 0)
            ? new Guid(p.secondaryServiceUuid)
            : null,
        descriptors = p.descriptors
            .map((d) => new BluetoothDescriptor.fromProto(d))
            .toList(),
        properties = new CharacteristicProperties.fromProto(p.properties),
        _value = BehaviorSubject.seeded(p.value);

so they need to be able to access the last value from a global store

But also, I think the design of global + tokens makes more sense.

I know theres a lot of breaking changes we could make, but I try not to break more than ~1 thing at a time. This makes it easier for most users to update.

Another thing to keep in mind, verbose logs.

this $SERVICE_UUID:$SERVICE_INDEX/$CHARACTERISTIC_UUID:$CHARACTERISTIC_INDEX style string sounds like it would be more annoying for viewing the verbose logs, which a lot of users - and me - find important. So whatever solution you make, keep the verbose logs legible.

I think adding index to Guid is still the most obvious solution that meets all criteria, without blowing up the code base to add a feature almost no one cares about.

This is ending up a slightly bigger change than anticipated but I'll get it finished and you can consider the option. So far looking like a pretty huge reduction in code bloat.

There is possibly a pretty neat solution to not breaking the token style construction with something like:

  factory BluetoothCharacteristic({
    DeviceIdentifier remoteId,
    Guid this.serviceUuid,
    Guid this.characteristicUuid,
    Guid? primaryServiceUuid,
  }) {
    return FlutterBluePlus.findCharacteristic(remoteId, serviceUuid, primaryServiceUuid, characteristicUuid);
  }

I think adding index to Guid is still the most obvious solution that meets all criteria, without blowing up the code base.

I don't doubt that this is the simplest solution but I think it's important to ensure that implementation details are not leaking into the public API and complicating things in the future. If we were to do this would a Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", null) be equivalent to Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)?

If we were to do this would a Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", null) be equivalent to Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)?

No. They are not equivalent.

serviceUuid = Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", null)

this refers to the first service with uuid = a87ad7d7-b3a9-4a80-a34e-300e7398c92c

serviceUuid = Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)

this refers to the second service with uuid = a87ad7d7-b3a9-4a80-a34e-300e7398c92c

If users want to compare uuid, they will need to compare guid.uuid instead.

for most users, its a completely transparent change, since this feature is rarely important. which is exactly what we want.

only when a users specifically wants to refer to the second service of a given uuid will they need to use Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)

instead of representing a UUID, GUID now represents a specific svc/chr/desc.

ah okay,

the first thing we should do is to fix the existing code then

maybe let's call it 'parentServiceUuid'

that said, it would probably be a list, because it could be included by multiple parents i assume

or i guess its intended to treat each parent/child pair as a separate service.

and we should use the isPrimary/isSecondary flags from the host OS, and copy them into the BmService struct like they used to be

and add docs to the codebase with explanations of all this stuff

my product launched this week, so i won't have time for awhile

How about we use the pointer address of the CBCharacteristic/CBService instances, (optionally together with the hash), to uniquely identify each one? This will be easier than finding the index. Then we flatten the services list with an NSSet - so we simply find the service with the matching pointer. When didModifyServices gets called we remove from the set.

pointer seems like a good idea

except i'm not sure how a user would instantiate it themself then before discoverServices

maybe we should just break that use case

i imagine it's used to do things like create chr listeners before you connect, for simpler code

or we can just assume null pointer is the first instance and not support that feature for included services

on second thought

index seems better

it's more helpful for debugging too

I think if we allow the index parameter to be null, and only filter by it if it is not null, we will solve 99% of that use case.

So if you do devices.servicesList.firstWhere((s) => s.uuid == ...).characteristics.firstWhere((c) => c.uuid == ...) you are given a concrete characteristic that is fully indexed, whereas if you initialize it with BluetoothCharacteristic(characteristicUuid: ..., serviceUuid: ...) it's anonymous and will match the first service and characteristic.

If someone needs to do multi-UUID (service or characteristic) setups they should use discovery mode. This is the same logic as BluetoothGatt.getService(UUID).

Indices will break if services are inserted/removed at an earlier position in the table, whereas pointers should remain valid.

Okay, sure, lets do ptr.

but keep it null unless there are multiple of the same service, chr, etc.

if there are multiple, lets give them all an pointer.

and I still think we should modify Guid.

looking reasonable. Some nice improvements, and some I think are not needed. Early feedback:

1. no need for BluetoothValueAttribute. Too much DRY. I want the code to be able to diverge easily. Since chr & desc are really not the same thing. In fact, I've considered changing desc.value to some other name many times. Plus having code that looks for either of the underlying OnXXXExent types is not so clean.

2. your changes for this code may be fewer lines, but they are harder to read & understand then before. Keeping the separate hasNotify, hasIndicate lines is easier to read & understand for the next person.

  bool get isNotifying {
    List<int> lastValue = cccd?.lastValue ?? [];
    return lastValue.isNotEmpty && (lastValue[0] & 0x03) > 0;
  }

Okay, I kept trying to give more feedback, but the PR is too big for me to wrap my head around right now.

There's a a lot of changes here.

A lot of it I do like, and would merge.

Now that you have a very good understanding of the FBP code, can you submit PR requests incrementally? A lot of these changes can be their own PR.

^ I'm sure you already realized this. And I know you've just been exploring whats possible.

AdvertisementData should not have a platform name. BLE advertisments do not have a platform name.

  AdvertisementData({
    required this.advName,
    required this.platformName,

I find this really hard to read.

// 00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678

It's easier to read like this:

{
 service_uuid: xxxx,
 characteristic_uuid: xxxx,
 descriptor_uuid: xxxx,
}

You might think "internal" representations don't matter. But it's not internal. We log these everywhere. Heck, I even expose these in my app when users look at the logs. So it's user facing too. Keep it as legible as before.

In other words, please turn on verbose logs, and make it look decent to read. Mind you, it needs to be performant too. I once tried to pretty format everything, and performance tanked.

This was an accidental push to the PR as I had a reference to the fork@master, but yeah it's a current snapshot of WIP.

Okay, I kept trying to give more feedback, but the PR is too big for me to wrap my head around right now.

Yeah sorry it's not very pretty right now but I've had to get it to a working state as I have deadlines with our project.

Now that you have a very good understanding of the FBP code, can you submit PR requests incrementally? A lot of these changes can be their own PR.

Can do my best to break up changes into their own commits. Maybe if you could comment in the code the areas you are 100% happy with can start extracting and rebasing piece by piece until its a smaller diff?

no need for BluetoothValueAttribute. Too much DRY. I want the code to be able to diverge easily.

Personally suggest maximum DRY until the divergence actually occurs. It's pretty easy to un-abstract if the need arises, but without great care the duplicated code can quickly balloon to a difficult to manage state.

Since chr & desc are really not the same thing. In fact, I've considered changing desc.value to some other name many times.

As far as the BLE spec is concerned they are almost identical (see 4.8 CHARACTERISTIC VALUE READ and 4.12 CHARACTERISTIC DESCRIPTORS), and the descriptor value is referred to as such. What were your thoughts behind changing it?

Plus having code that looks for either of the underlying OnXXXExent types is not so clean.

Note that prior to these changes the same functions were re-parsing the events everywhere:

Stream<List<int>> get lastValueStream => FlutterBluePlus._methodStream.stream
      .where((m) => m.method == "OnCharacteristicReceived" || m.method == "OnCharacteristicWritten")
      .map((m) => m.arguments)
      .map((args) => BmCharacteristicData.fromMap(args))

So I figured delegating discrimination to the .where((e) => e.attribute == this) was a pretty elegant solution as it's 0 cost.

your changes for this code may be fewer lines, but they are harder to read & understand then before. Keeping the separate hasNotify, hasIndicate lines is easier to read & understand for the next person.

bool get isNotifying {
   List<int> lastValue = cccd?.lastValue ?? [];
   return lastValue.isNotEmpty && (lastValue[0] & 0x03) > 0;
 }```

Agree it's rather vague as is, how about lastValue[0] & (CCCD_BIT_NOTIFY | CCCD_BIT_INDICATE)?

I do believe as a general principle that brevity like this is beneficial and makes classes more readable:

bool get isNotifying => (cccd?.lastValue?._firstOrNull ?? 0) & (CCCD_BIT_NOTIFY | CCCD_BIT_INDICATE) > 0

AdvertisementData should not have a platform name. BLE advertisments do not have a platform name.

Ok so this should live in ScanResult rather than AdvertisementData then, must have accidentally put it there as I was reducing reliance on the Bm*** classes.

I find this really hard to read.
// 00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678

Yeah this is not strictly the final form, was just convenient for now, however...

Mind you, it needs to be performant too. I once tried to pretty format everything, and performance tanked.

The fact that we can't pretty print everything means that you will get { service_id: 00000000-0000-0000-0000-000000000000:12345678, characteristic_id: 00000000-0000-0000-0000-000000000000:12345678, descriptor_id: 00000000-0000-0000-0000-000000000000:12345678 } which isn't exactly great when your screen gets spammed with UUID characters.

Would you be open to this? 20% less characters but still comparatively quite readable:

[ S@00000000-0000-0000-0000-000000000000:12345678, C@00000000-0000-0000-0000-000000000000:12345678, D@00000000-0000-0000-0000-000000000000:12345678 ]

Either way I'm happy with some separation of them to cut out the string parsing on native side.

too much to discuss at one time.

let's just do it one PR at a time.

class OnDescriptorWrittenEvent with GetDeviceMixin, GetAttributeValueMixin, GetDescriptorMixin, GetExceptionMixin {

this is not easier to understand than just having the very very little code it replaces in the class.

too much DRY.

Having code duplicate is not bad. It often leads to easier to understand code, easier to modify code, and flatter class hierarchies.

When designing code "look a common pattern! I can replace all these lines into a single new concept!" it not a good guiding principal. Instead "this code is too complicated, and I can make it easier to understand", is all we should care about.

Edit: sorry if I sound preachy here ^. Just my point of voice. DRY often leads to more complicated code, for little benefit.

I think it would be splitting hairs here to suggest that a mixin is somehow significantly less readable than the line it replaced - when that line is copied 2-10x in the file. Could equally argue the mixins allow one to focus on what is unique to each event and really this is the perfect use-case for them.

It is however an issue of maintainability when a simple change in the parameters of a class requires 10 identical changes elsewhere. This introduces a very real risk of missing one of the required changes and makes it more difficult to understand and review changes.

To take a concrete example: 2064c18, the line .where((p) => p.primaryServiceUuid is added 7 times in this relatively simple change. Would an external contributor remember to put in all 7? From viewing the diff alone it's impossible to assess that.

There is of course no single right answer on the degree to which the code should be abstracted, but I can tell you from my perspective as an external contributor that in its current form the library is quite difficult to work with primarily due to the amount of duplicated code. Every time I see a duplicated block I must ask myself why isn't this abstracted if it's doing the exact same thing, is there something unique about it?

In addition to that, while it is merely a metric - when I see files with hundreds or even thousands of lines of code it is very daunting. The 700 lines this branch removed while maintaining the same functionality is 700 less lines I have to understand as a contributor. Look especially at the before and after of service/char/desc classes, I can now get the gist of what they are responsible for without even having to scroll.

I had to google to remember the rules of mixins. Thats certainly not more readable than a few extra lines. Eventually you end up with the hell of C++, where you need to understand huge textbooks to read 100 lines of code.

mixins: "At first, I found this concept somewhat difficult to understand" https://medium.com/flutter-community/dart-what-are-mixins-3a72344011f3

Simply put, agree to disagree.

The code is not perfect, ofc. Some refactors are overdue. We dont need to introduce the concept of mixins to this codebase.

readability > "maintainability"

RE: contributors & "700" less lines of code.

Lines of code is not a good metric.

I gave up on flutter_reactive_ble because it was too abstracted, and used too many swift language features that I didn't want to have to learn, in order to reduce lines of code.

look at this file: https://github.com/PhilipsHue/flutter_reactive_ble/blob/master/packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Central.swift

^ look at connect (line 145). Does it makes any sense to you?

or this file: https://github.com/PhilipsHue/flutter_reactive_ble/blob/master/packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Tasks/CharacteristicWrite/CharacteristicWriteTaskSpec.swift

this file:
https://github.com/PhilipsHue/flutter_reactive_ble/blob/master/packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Tasks/CharacteristicWrite/CharacteristicWriteTaskController.swift

And look where they ended up. They went from the most popular BLE library, to the 2nd most popular. And soon to be the 3rd.

You should look at their code and see if it would have been easier for you to contribute to.

It does the same exact stuff as this library.

Keeping code "flat" is important.

that said this code base is not perfect either. the obj-c & java files are too big. They're not complicated. But they're too big now. Just some simple util files needed.

anyway, still, lets just go PR by PR, if you want to merge.

We dont need to agree on everything.

I had to google to remember the rules of mixins.
mixins: "At first, I found this concept somewhat difficult to understand"

..."until I realized how powerful it was."?

Instead of arguing the merits of not introducing mixins to the codebase because you are not familiar with them, why not learn how they work and benefit from their use? Every language feature is difficult until you learn how to use it.

readability > "maintainability"

Maintainability is no less important than readability. If I am writing professional software I need to have confidence that I can easily develop new features and fix bugs in the library.

Keeping code "flat" is important.

Repeating identical code because it's technically quicker to read in one place vs over multiple classes does not make it flat or readable. Abstractions are useful and necessary and there is a lot more nuance to it than flat hierarchy = better.

BluetoothAttribute => BluetoothValueAttribute => BluetoothCharacteristic is truly a textbook example of good class hierarchy, perfectly modelling the underlying concepts from GATT.

And look where they ended up. They went from the most popular BLE library, to the 2nd most popular. And soon to be the 3rd.

The external API of this library is better and it is more actively maintained, that is why it is popular, not because it lacks abstractions.

anyway, still, lets just go PR by PR, if you want to merge.
We dont need to agree on everything.

No, we certainly don't. I have obviously been willing to devote a lot of time to these changes and this discussion, and I appreciate the same from you. I'm not adamanant on mixins or any part of this change in particular, and I hope we are just getting caught up on minor details.

That said we will need to reach a reasonable consensus if I am going to dedicate further time breaking this up. Have no problem skipping out on some of the functional changes but quite frankly I will need a lot more commitment than "some simple util files".

Instead of arguing the merits of not introducing mixins to the codebase because you are not familiar with them, why not learn how they work and benefit from their use? Every language feature is difficult until you learn how to use it.

The benefit here is too small.

And it's not really about me. It's about the new Flutter developer learning Dart for the first time.

abstract class BluetoothValueAttribute extends BluetoothAttribute {

  /// this variable is updated:
  ///   - anytime `read()` is called
  ///   - anytime `write()` is called
  ///   - anytime a notification arrives (characteristics, if subscribed)
  ///   - when the device is disconnected it is cleared
  List<int> get lastValue => _lastValue;

I'd probably phrase it like this: - anytime a notification arrives, if subscribed (only applies to characteristics)

But still, needing to keep these docs generic across descriptors & characteristics is not something I want to care about. There's really no reason for DRY here, since the code/concepts do not need to stay in sync. It just happens to be in sync right now.

I've only given DRY push back for BluetoothValueAttribute and the Mixins.

It's really a small amount of pushback, given the size of this PR. If you're okay not merging those, we can move on.

To be clear, I'm not saying its bad, they're reasonable. It's just not the tradeoffs i'm looking for in this codebase.

And it's not really about me. It's about the new Flutter developer learning Dart for the first time.

For your consideration: a developer that is just learning the language is highly unlikely to be poking around the internals of a BLE library, let alone contributing changes.

But still, needing to keep these docs generic across descriptors & characteristics is not something I want to care about. There's really no reason for DRY here, since the code/concepts do not need to stay in sync. It just happens to be in sync right now.

It would perhaps help to understand what changes you are anticipating to them - as according to the GATT spec these concepts are very much in sync.

Surely keeping one or two extra lines of documentation in sync is easier than keeping double the code and double the documentation in sync?

Anyway, good that we are mostly on the same page then, I'll get started on the events refactor.

It would perhaps help to understand what changes you are anticipating to them - as according to the GATT spec these concepts are very much in sync.

For example, on iOS there are restrictions which descriptors can be written to. These belong in the DESC docs, not CHR docs. And these restrictions can change any time.

@nebkat , have you ever read this article? https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction

In this pr we see exactly the problem this article is describing already starting to happen. The docs having new "only applies to characteristics" conditionals, parentAttribute is now added to descriptors, but not actually used (and should be called parentService).

Only deduplicate (i.e. add abstractions) when the value of it is high. When you need a single source of information. etc.

@chipweinberger Yes, I have. It's a good article and it makes a very valid point - however it's important not to take such opinion pieces at face value without considering the specific context.

You seem to be under the impression that avoiding abstraction is implicitly better because there is a risk of it being wrong, which is IMO looking at the problem backwards. A flat hierarchy should rarely be the goal. It is one of the possible solutions when direct abstracting doesn't work.

Thus the important question is whether or not a given abstraction is actually wrong. If you look at the example given in the article:

Programmer B feels honor-bound to retain the existing abstraction, but since isn't exactly the same for every case, they alter the code to take a parameter, and then add logic to conditionally do the right thing based on the value of that parameter.

What was once a universal abstraction now behaves differently for different cases.

I absolutely agree that if BluetoothValueAttribute itself started exhibiting different behaviors depending on its derivation it would cease to be a good abstraction - at which point duplication of the relevant feature is preferrable. But as it stands:

• Functionality is identical between both
• It closely models the concept that exists in GATT
• There are more lines in common between them than there are in the entire BluetoothService class

The single extra line of documentation needed to clarify that notifications are only relevant to characteristics is not a "parameter" as described above. It's totally normal to document different behaviors of derived classes (that's kinda the point of deriving in the first place!). Should Flex not exist because some of its properties provide extra context for the derived classes?

You also mention the iOS restrictions on writing certain descriptors - but the BluetoothDescriptor class still exists and has its own write method so I'm not sure how that is relevant?

The point the article is making is that you shouldn't be afraid to remove abstractions when they are no longer serving their purpose. I suggest you read some of the comments on this article, as well as the discussions here.

parentAttribute is now added to descriptors, but not actually used (and should be called parentService)

Figure 6.3: GATT-Based Profile hierarchy from the spec:

As you can see service is the parent of characteristic is the parent of descriptor in the GATT hierarchy.

That's all fair.

But at the end of the day, it's my project, and I find duplication easier to reason about.

I try to avoid abstracted code. It's my preference.

And oh, okay. I see how you intend to use parent now.

I'm not sure if we really need that.

I've thought about adding it before tho. But seems unecessary.

I dont think this is correct. If you have a parent, you're not primary.

From Bluetooth Core Spec v5.4 | Vol 3, Part G | Page 1472:

So primary → primary is possible, as well as primary → secondary → secondary.

The include is simply a link to another service definition in the table which exists independently. The only difference between primary and secondary, aside from semantics, is that only primary services show up in the Discover All Primary Services sub-procedure, whereas secondary have to be manually requested, however the Bluetooth stacks on Android/iOS already handle this for us.

The good news is on Android it appears that all the secondary services are included in BluetoothGatt.getServices() anyway - so they can always be uniquely identified by their UUID and their handle without the need for hierarchy information. I presume iOS is the same, will find out hopefully later today.

it would be great to fix this in the current FBP.